-
Notifications
You must be signed in to change notification settings - Fork 470
Fix issue #308: Find py files in MCPForUnityTools and version.txt #309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…n.txt This allows for auto finding new tools. A good dir on a custom tool would look like this: CustomTool/ ├── CustomTool.MCPEnabler.asmdef ├── CustomTool.MCPEnabler.asmdef.meta ├── ExternalAssetToolFunction.cs ├── ExternalAssetToolFunction.cs.meta ├── external_asset_tool_function.py ├── external_asset_tool_function.py.meta ├── version.txt └── version.txt.meta CS files are left in the tools folder. The asmdef is recommended to allow for dependency on MCPForUnity when MCP For Unity is installed: asmdef example { "name": "CustomTool.MCPEnabler", "rootNamespace": "MCPForUnity.Editor.Tools", "references": [ "CustomTool", "MCPForUnity.Editor" ], "includePlatforms": [ "Editor" ], "excludePlatforms": [], "allowUnsafeCode": false, "overrideReferences": false, "precompiledReferences": [], "autoReferenced": true, "defineConstraints": [], "versionDefines": [], "noEngineReferences": false }
WalkthroughThe changes implement tool synchronization between Unity project and MCP server, introducing version tracking per MCPForUnityTools folder, incremental copying of project tools into server directories, stale folder cleanup, and subdirectory-based tool auto-discovery. Control flow updated to check tool versions during initialization and copy tools during server installation/rebuild. Changes
Sequence Diagram(s)sequenceDiagram
participant PackageDetector
participant ServerInstaller
participant ProjectTools as Project Tools<br/>(MCPForUnityTools)
participant ServerTools as Server Tools<br/>(destToolsDir)
participant ToolsInit as tools/__init__.py
rect rgb(200, 220, 240)
Note over PackageDetector,ServerTools: Initialization & Version Check
PackageDetector->>ProjectTools: Scan for MCPForUnityTools folders
PackageDetector->>ServerTools: Compare version.txt files
alt Versions mismatch or missing
PackageDetector->>ServerInstaller: toolsNeedUpdate = true
end
end
rect rgb(240, 220, 200)
Note over ServerInstaller,ServerTools: Server Install/Rebuild
ServerInstaller->>ProjectTools: CopyUnityProjectTools()
ProjectTools-->>ServerInstaller: Copy .py files to per-folder<br/>subdirectories
ServerInstaller->>ServerTools: Update version.txt tracking
ServerInstaller->>ServerTools: CleanupStaleToolFolders()
end
rect rgb(220, 240, 200)
Note over ToolsInit,ServerTools: Tool Discovery
ToolsInit->>ServerTools: Auto-discover top-level tools
ToolsInit->>ServerTools: Auto-discover subdirectory tools
ToolsInit-->>ToolsInit: Import modules with warnings<br/>on failures
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span three files with mixed complexity: new version tracking logic with path traversal and file I/O, tool copying with directory management, and extended module discovery. While individual functions are relatively straightforward, the interconnected version tracking, per-folder identifier logic, and error handling across multiple entry points require careful verification of correctness and edge cases. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (5)
MCPForUnity/Editor/Helpers/PackageDetector.cs (2)
135-137
: Limit scan scope to Assets/Packages to avoid heavy project-wide traversal.Scanning the entire project root can walk Library, Temp, etc. Prefer Assets and Packages only.
Apply this diff:
- // Find all MCPForUnityTools folders in project - var toolsFolders = System.IO.Directory.GetDirectories(projectRoot, "MCPForUnityTools", System.IO.SearchOption.AllDirectories); + // Find all MCPForUnityTools folders under Assets and Packages only + var toolsFolders = new System.Collections.Generic.List<string>(); + string assetsDir = System.IO.Path.Combine(projectRoot, "Assets"); + if (System.IO.Directory.Exists(assetsDir)) + toolsFolders.AddRange(System.IO.Directory.GetDirectories(assetsDir, "MCPForUnityTools", System.IO.SearchOption.AllDirectories)); + string packagesDir = System.IO.Path.Combine(projectRoot, "Packages"); + if (System.IO.Directory.Exists(packagesDir)) + toolsFolders.AddRange(System.IO.Directory.GetDirectories(packagesDir, "MCPForUnityTools", System.IO.SearchOption.AllDirectories));
181-213
: Deduplicate GetToolsFolderIdentifier to avoid drift.This logic is duplicated here and in ServerInstaller. Prefer a single source of truth.
- Change ServerInstaller.GetToolsFolderIdentifier(...) to internal static.
- Use that here and remove this duplicate method.
Proposed changes:
In ServerInstaller.cs (make method internal):
- private static string GetToolsFolderIdentifier(string toolsFolderPath) + internal static string GetToolsFolderIdentifier(string toolsFolderPath)In PackageDetector.cs (remove this method and call ServerInstaller.GetToolsFolderIdentifier(folder) where needed).
MCPForUnity/Editor/Helpers/ServerInstaller.cs (3)
421-423
: Reduce scan scope to Assets/Packages for performance.Avoid traversing Library/Temp etc. on each domain reload.
Apply this diff:
- // Find all MCPForUnityTools folders - var toolsFolders = Directory.GetDirectories(projectRoot, "MCPForUnityTools", SearchOption.AllDirectories); + // Find all MCPForUnityTools folders under Assets and Packages only + var toolsFolders = new List<string>(); + var assetsDir = Path.Combine(projectRoot, "Assets"); + if (Directory.Exists(assetsDir)) + toolsFolders.AddRange(Directory.GetDirectories(assetsDir, "MCPForUnityTools", SearchOption.AllDirectories)); + var packagesDir = Path.Combine(projectRoot, "Packages"); + if (Directory.Exists(packagesDir)) + toolsFolders.AddRange(Directory.GetDirectories(packagesDir, "MCPForUnityTools", SearchOption.AllDirectories));
494-531
: Expose GetToolsFolderIdentifier for reuse.Make this helper internal so PackageDetector can reuse it and remove duplicate logic.
Apply this diff:
- private static string GetToolsFolderIdentifier(string toolsFolderPath) + internal static string GetToolsFolderIdentifier(string toolsFolderPath)
483-487
: Optional: log skipped tools for visibility.You track skippedCount but don't surface it.
Apply this diff:
if (copiedCount > 0) { McpLog.Info($"Copied {copiedCount} Unity project tool(s) to server"); } + else if (skippedCount > 0) + { + McpLog.Info($"All Unity project tools up-to-date ({skippedCount} skipped)"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Helpers/PackageDetector.cs
(2 hunks)MCPForUnity/Editor/Helpers/ServerInstaller.cs
(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
MCPForUnity/Editor/Helpers/PackageDetector.cs (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (3)
ServerInstaller
(12-836)GetServerPath
(117-120)GetToolsFolderIdentifier
(498-531)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
MCPForUnity/Editor/Helpers/PackageDetector.cs (1)
GetToolsFolderIdentifier
(184-213)
🔇 Additional comments (2)
MCPForUnity/Editor/Helpers/PackageDetector.cs (1)
26-29
: Good: update path now considers tool version drift.This ensures domain-reload updates when project tool versions change.
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
450-465
: Namespace copied tools to avoid collisions
Flat-copying all.py
files intotools
allows same-named files from differentfolderIdentifier
to overwrite each other. Choose one:
- Copy into per-source subfolders:
(Ensure your server loads tools recursively.)var targetDir = Path.Combine(destToolsDir, folderIdentifier); Directory.CreateDirectory(targetDir); var destFile = Path.Combine(targetDir, fileName);- Or detect duplicates before
File.Copy
(e.g.if (!seen.Add(fileName) || File.Exists(destFile)) log and skip.
)Confirm whether the server’s tool discovery supports nested directories.
…lderIdentifier was duplicated>)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
410-492
: Prevent copy failures, avoid filename collisions, and limit scan scope.
- Ensure dest tools dir exists before File.Copy.
- Flat-copying .py into a shared tools root risks overwriting between multiple MCPForUnityTools folders (same filenames). Copy into a per-folder subdirectory keyed by folderIdentifier.
- Limit scan to Assets/ and Packages/ to avoid deep recursion into Library/.git and speed up. Also clean stale .py removed upstream.
Apply this diff:
private static void CopyUnityProjectTools(string destToolsDir) { try { // Get Unity project root string projectRoot = Directory.GetParent(Application.dataPath)?.FullName; if (string.IsNullOrEmpty(projectRoot)) { return; } + // Ensure destination tools directory exists + Directory.CreateDirectory(destToolsDir); + - // Find all MCPForUnityTools folders - var toolsFolders = Directory.GetDirectories(projectRoot, "MCPForUnityTools", SearchOption.AllDirectories); + // Find all MCPForUnityTools folders (limit to Assets/ and Packages/) + var searchRoots = new[] { Path.Combine(projectRoot, "Assets"), Path.Combine(projectRoot, "Packages") } + .Where(Directory.Exists); + var toolsFolders = searchRoots + .SelectMany(root => Directory.GetDirectories(root, "MCPForUnityTools", SearchOption.AllDirectories)) + .ToArray(); int copiedCount = 0; int skippedCount = 0; foreach (var folder in toolsFolders) { // Generate unique identifier for this tools folder based on its parent directory structure // e.g., "MooseRunner_MCPForUnityTools" or "MyPackage_MCPForUnityTools" string folderIdentifier = GetToolsFolderIdentifier(folder); - string versionTrackingFile = Path.Combine(destToolsDir, $"{folderIdentifier}_version.txt"); + string destToolFolder = Path.Combine(destToolsDir, folderIdentifier); + string versionTrackingFile = Path.Combine(destToolFolder, "_version.txt"); // Read source version string sourceVersionFile = Path.Combine(folder, "version.txt"); string sourceVersion = ReadVersionFile(sourceVersionFile) ?? "0.0.0"; // Read installed version (tracked separately per tools folder) string installedVersion = ReadVersionFile(versionTrackingFile); // Check if update is needed (version different or no tracking file) bool needsUpdate = string.IsNullOrEmpty(installedVersion) || sourceVersion != installedVersion; if (needsUpdate) { // Get all .py files (excluding __init__.py) var pyFiles = Directory.GetFiles(folder, "*.py") .Where(f => !Path.GetFileName(f).Equals("__init__.py", StringComparison.OrdinalIgnoreCase)); + // Ensure per-tool destination exists + Directory.CreateDirectory(destToolFolder); + + // Cleanup stale .py files that were removed upstream + try + { + var sourceNames = new HashSet<string>(pyFiles.Select(f => Path.GetFileName(f)), StringComparer.OrdinalIgnoreCase); + foreach (var destPy in Directory.GetFiles(destToolFolder, "*.py")) + { + if (!sourceNames.Contains(Path.GetFileName(destPy))) + { + File.Delete(destPy); + } + } + } + catch (Exception cleanupEx) + { + McpLog.Warn($"Failed to cleanup stale files for {folderIdentifier}: {cleanupEx.Message}"); + } + foreach (var pyFile in pyFiles) { string fileName = Path.GetFileName(pyFile); - string destFile = Path.Combine(destToolsDir, fileName); + string destFile = Path.Combine(destToolFolder, fileName); try { File.Copy(pyFile, destFile, overwrite: true); copiedCount++; McpLog.Info($"Copied Unity project tool: {fileName} from {folderIdentifier} (v{sourceVersion})"); } catch (Exception ex) { McpLog.Warn($"Failed to copy {fileName}: {ex.Message}"); } } // Update version tracking file try { File.WriteAllText(versionTrackingFile, sourceVersion); } catch (Exception ex) { McpLog.Warn($"Failed to write version tracking file for {folderIdentifier}: {ex.Message}"); } } else { skippedCount++; } } if (copiedCount > 0) { McpLog.Info($"Copied {copiedCount} Unity project tool(s) to server"); } } catch (Exception ex) { McpLog.Warn($"Failed to scan Unity project for tools: {ex.Message}"); } }Note: If tools rely on package-style relative imports, consider copying their
__init__.py
into the per-folder subdir to preserve package semantics. Please confirm expected import pattern.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
498-531
: Consolidate GetToolsFolderIdentifier into a shared helper
Extract the duplicate logic from ServerInstaller.cs and PackageDetector.cs into a single utility method to keep identifier rules consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MCPForUnity/Editor/Helpers/PackageDetector.cs
(2 hunks)MCPForUnity/Editor/Helpers/ServerInstaller.cs
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Editor/Helpers/PackageDetector.cs
🔇 Additional comments (2)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (2)
61-64
: Good integration point for project tools.Running tools copy independently of server version makes sense. No issues at the call site.
597-600
: Rebuild path also wiring tools copy — good.Hooking tools copy into rebuild keeps server and tools in sync.
… GetToolsFolderIdentifier, fix tools copy dir, and limit scan scope
So all ready to go if any one who approves likes it. :) |
So now pycache fix is done. Used the _skipDirs as that sounds about right ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs
(3 hunks)
🔇 Additional comments (4)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (4)
55-63
: LGTM!The integration of
CopyUnityProjectTools
intoEnsureServerInstalled
is correct. The method is called after server installation/update, and the comment clearly indicates that tool copying runs independently of server version updates, which aligns with the PR objectives.
534-574
: LGTM!The cleanup logic correctly removes stale tool subdirectories that no longer have corresponding source folders in the Unity project. The use of
goto
on lines 551 and 567 is an acceptable C# idiom for breaking out of nested loops in this context.
580-613
: LGTM!The identifier generation logic correctly walks up the directory tree to find a distinctive parent folder. The method appropriately stops at
Assets
orPackages
boundaries and provides reasonable fallbacks for edge cases.
679-681
: LGTM!The integration of
CopyUnityProjectTools
intoRebuildMcpServer
ensures that Unity project tools remain synchronized after a server rebuild, maintaining consistency with theEnsureServerInstalled
flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs
(3 hunks)
🔇 Additional comments (5)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (5)
404-404
: LGTM!The skip list appropriately covers Python virtual environments, cache directories, and version control artifacts that should be excluded during server installation and cleanup operations.
62-63
: LGTM!The integration of
CopyUnityProjectTools
after server installation ensures that Unity project tools are synchronized independently of server version updates, which aligns with the PR objectives for automatic tool deployment.
686-688
: LGTM!Consistent integration pattern with
EnsureServerInstalled
, ensuring tools are copied after server rebuild operations.
411-536
: LGTM!The
CopyUnityProjectTools
implementation is comprehensive and well-structured:
- ✅ Properly searches limited roots (Assets, Packages, PackageCache) to avoid performance issues
- ✅ Creates destination directory before operations (addresses previous review comment)
- ✅ Per-folder versioning with version.txt tracking enables conditional updates
- ✅ Validates and skips folders without .py files (addresses previous review comment)
- ✅ Robust error handling with informative logging
- ✅ Cleanup of stale tool folders ensures consistency
The version comparison using strict string equality (line 476) is appropriate for this use case where exact version matching determines when to copy tools.
541-581
: LGTM!The cleanup logic correctly removes stale tool folders while preserving Python cache and virtual environment directories. The error handling ensures that failures to delete individual directories don't halt the entire cleanup process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
455-489
: Don’t create per-folder dirs when no .py files; add to active set only if a tool exists.Currently the subdir is created and left empty when no .py files are found. Move enumeration before creating the subdir and before adding to activeFolderIdentifiers.
Apply this diff:
- // Generate unique identifier for this tools folder based on its parent directory structure - // e.g., "MooseRunner_MCPForUnityTools" or "MyPackage_MCPForUnityTools" - string folderIdentifier = GetToolsFolderIdentifier(folder); - activeFolderIdentifiers.Add(folderIdentifier); - - // Create per-folder subdirectory in destToolsDir - string destFolderSubdir = Path.Combine(destToolsDir, folderIdentifier); - Directory.CreateDirectory(destFolderSubdir); - - string versionTrackingFile = Path.Combine(destFolderSubdir, "version.txt"); - - // Read source version - string sourceVersionFile = Path.Combine(folder, "version.txt"); - string sourceVersion = ReadVersionFile(sourceVersionFile) ?? "0.0.0"; - - // Read installed version (tracked separately per tools folder) - string installedVersion = ReadVersionFile(versionTrackingFile); - - // Check if update is needed (version different or no tracking file) - bool needsUpdate = string.IsNullOrEmpty(installedVersion) || sourceVersion != installedVersion; - - if (needsUpdate) - { - // Get all .py files (excluding __init__.py) - var pyFiles = Directory.GetFiles(folder, "*.py") - .Where(f => !Path.GetFileName(f).Equals("__init__.py", StringComparison.OrdinalIgnoreCase)); - - // Skip folders with no .py files - if (!pyFiles.Any()) - { - skippedCount++; - continue; - } + // Get all .py files (excluding __init__.py) + var pyFiles = Directory.GetFiles(folder, "*.py") + .Where(f => !Path.GetFileName(f).Equals("__init__.py", StringComparison.OrdinalIgnoreCase)); + + // Skip folders with no .py files (don't add to active set and don't create subdir) + if (!pyFiles.Any()) + { + skippedCount++; + continue; + } + + // Generate identifier only for valid tool folders and mark active + string folderIdentifier = GetToolsFolderIdentifier(folder); + activeFolderIdentifiers.Add(folderIdentifier); + + string destFolderSubdir = Path.Combine(destToolsDir, folderIdentifier); + string versionTrackingFile = Path.Combine(destFolderSubdir, "version.txt"); + + // Read versions + string sourceVersionFile = Path.Combine(folder, "version.txt"); + string sourceVersion = ReadVersionFile(sourceVersionFile) ?? "0.0.0"; + string installedVersion = ReadVersionFile(versionTrackingFile); + + bool needsUpdate = string.IsNullOrEmpty(installedVersion) || sourceVersion != installedVersion; + + if (needsUpdate) + { + Directory.CreateDirectory(destFolderSubdir);
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (2)
435-447
: Deduplicate “Packages” vs “Library/PackageCache” results.Scanning both can find the same tool twice. Prefer the Packages path when both exist; skip PackageCache duplicates.
Would you like me to propose a small de-dupe map keyed by package folder name (preferring “Packages/” over “Library/PackageCache/”)?
480-489
: Verify intent: excluding init.py and not copying subdirectories.If tools use packages or one‑level submodules, skipping init.py and not copying subdirs may break imports.
- Option A: keep single‑file tool policy; document it clearly.
- Option B: copy one‑level subdirectories (skip pycache, venvs) and include init.py when present.
Do you want a patch for Option B?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs
(3 hunks)
🔇 Additional comments (2)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (2)
422-424
: LGTM: ensure destination tools dir exists.Creating destToolsDir prevents File.Copy failures on fresh installs.
61-64
: LGTM: project tools copied during install and rebuild.Hooks are placed correctly after server copy.
Also applies to: 686-689
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ict to managed names.
…into fix-issue-308
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
596-629
: Critical: Address unresolved identifier collision and PackageCache issues.The previous review identified two problems that remain unfixed:
PackageCache not handled: For paths like
Library/PackageCache/com.example@1.0.0/Editor/MCPForUnityTools
, the code doesn't recognizePackageCache
as a valid root, falling back to the genericEditor_MCPForUnityTools
identifier.Collision risk: Multiple packages under the same parent produce identical identifiers. For example:
Assets/Plugins/Package1/Editor/MCPForUnityTools
→Assets_Plugins_MCPForUnityTools
Assets/Plugins/Package2/Editor/MCPForUnityTools
→Assets_Plugins_MCPForUnityTools
These collisions cause tools to overwrite each other.
Apply the diff from the previous review to fix both issues:
internal static string GetToolsFolderIdentifier(string toolsFolderPath) { try { - // Get parent directory name (e.g., "Editor" or package name) - DirectoryInfo parent = Directory.GetParent(toolsFolderPath); - if (parent == null) return "MCPForUnityTools"; - - // Walk up to find a distinctive parent (Assets/PackageName or Packages/PackageName) - DirectoryInfo current = parent; - while (current != null) - { - string name = current.Name; - DirectoryInfo grandparent = current.Parent; - - // Stop at Assets, Packages, or if we find a package-like structure - if (grandparent != null && - (grandparent.Name.Equals("Assets", StringComparison.OrdinalIgnoreCase) || - grandparent.Name.Equals("Packages", StringComparison.OrdinalIgnoreCase))) - { - return $"{grandparent.Name}_{name}_MCPForUnityTools"; - } - - current = grandparent; - } - - // Fallback: use immediate parent - return $"{parent.Name}_MCPForUnityTools"; + // toolsFolderPath → .../<PackageFolder>/Editor/MCPForUnityTools + var editorDir = Directory.GetParent(toolsFolderPath); // Editor + var packageDir = editorDir?.Parent; // <PackageFolder> + if (packageDir == null) return "MCPForUnityTools"; + + // Find root (Assets / Packages / PackageCache) + DirectoryInfo current = packageDir; + while (current?.Parent != null) + { + var p = current.Parent; + if (p.Name.Equals("Assets", StringComparison.OrdinalIgnoreCase) || + p.Name.Equals("Packages", StringComparison.OrdinalIgnoreCase) || + p.Name.Equals("PackageCache", StringComparison.OrdinalIgnoreCase)) + { + return $"{p.Name}_{current.Name}_MCPForUnityTools"; + } + current = p; + } + + // Fallback: use package dir name + return $"{packageDir.Name}_MCPForUnityTools"; } catch { return "MCPForUnityTools"; } }This ensures unique identifiers by including the package folder name directly above Editor, preventing collisions between different packages under the same parent directory, and properly handling PackageCache locations.
🧹 Nitpick comments (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
462-465
: Consider deferring subdirectory creation until after version check.The subdirectory is created on line 464 before checking if an update is needed on line 478. For up-to-date folders, this performs unnecessary directory creation operations.
Apply this diff to defer directory creation:
string folderIdentifier = GetToolsFolderIdentifier(folder); activeFolderIdentifiers.Add(folderIdentifier); - // Create per-folder subdirectory in destToolsDir - string destFolderSubdir = Path.Combine(destToolsDir, folderIdentifier); - Directory.CreateDirectory(destFolderSubdir); - + string destFolderSubdir = Path.Combine(destToolsDir, folderIdentifier); string versionTrackingFile = Path.Combine(destFolderSubdir, "version.txt"); // Read source version string sourceVersionFile = Path.Combine(folder, "version.txt"); string sourceVersion = ReadVersionFile(sourceVersionFile) ?? "0.0.0"; // Read installed version (tracked separately per tools folder) string installedVersion = ReadVersionFile(versionTrackingFile); // Check if update is needed (version different or no tracking file) bool needsUpdate = string.IsNullOrEmpty(installedVersion) || sourceVersion != installedVersion; if (needsUpdate) { + // Create per-folder subdirectory in destToolsDir + Directory.CreateDirectory(destFolderSubdir); + // Get all .py files (excluding __init__.py)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (2)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (2)
462-465
: Avoid creating empty dest subdirs — create lazily after confirming .py files.Create destFolderSubdir only when pyFiles.Any() and an update is needed; prevents noise from empty folders.
- // Create per-folder subdirectory in destToolsDir - string destFolderSubdir = Path.Combine(destToolsDir, folderIdentifier); - Directory.CreateDirectory(destFolderSubdir); - - string versionTrackingFile = Path.Combine(destFolderSubdir, "version.txt"); + // Prepare paths; create dest subdir lazily + string destFolderSubdir = Path.Combine(destToolsDir, folderIdentifier); + string versionTrackingFile = Path.Combine(destFolderSubdir, "version.txt"); ... - // Skip folders with no .py files + // Skip folders with no .py files if (!pyFiles.Any()) { skippedCount++; continue; } + // Ensure destination subdir exists only when we actually copy + Directory.CreateDirectory(destFolderSubdir);Also applies to: 484-489
596-629
: Identifier collisions for PackageCache — derive stable, unique names (critical).Current logic doesn’t handle Library/PackageCache; many tools resolve to “Editor_MCPForUnityTools”, causing overwrites and bad cleanup.
- internal static string GetToolsFolderIdentifier(string toolsFolderPath) + internal static string GetToolsFolderIdentifier(string toolsFolderPath) { try { - // Get parent directory name (e.g., "Editor" or package name) - DirectoryInfo parent = Directory.GetParent(toolsFolderPath); - if (parent == null) return "MCPForUnityTools"; - - // Walk up to find a distinctive parent (Assets/PackageName or Packages/PackageName) - DirectoryInfo current = parent; - while (current != null) - { - string name = current.Name; - DirectoryInfo grandparent = current.Parent; - - // Stop at Assets, Packages, or if we find a package-like structure - if (grandparent != null && - (grandparent.Name.Equals("Assets", StringComparison.OrdinalIgnoreCase) || - grandparent.Name.Equals("Packages", StringComparison.OrdinalIgnoreCase))) - { - return $"{grandparent.Name}_{name}_MCPForUnityTools"; - } - - current = grandparent; - } - - // Fallback: use immediate parent - return $"{parent.Name}_MCPForUnityTools"; + // toolsFolderPath .../<PackageFolder>/Editor/MCPForUnityTools + var editorDir = Directory.GetParent(toolsFolderPath); // Editor + var packageDir = editorDir?.Parent; // <PackageFolder> + if (packageDir == null) return "MCPForUnityTools"; + + // Find root (Assets / Packages / PackageCache) + DirectoryInfo current = packageDir; + while (current?.Parent != null) + { + var p = current.Parent; + if (p.Name.Equals("Assets", StringComparison.OrdinalIgnoreCase) || + p.Name.Equals("Packages", StringComparison.OrdinalIgnoreCase) || + p.Name.Equals("PackageCache", StringComparison.OrdinalIgnoreCase)) + { + return $"{p.Name}_{current.Name}_MCPForUnityTools"; + } + current = p; + } + + // Fallback: use package dir name + return $"{packageDir.Name}_MCPForUnityTools"; } catch { return "MCPForUnityTools"; } }
🧹 Nitpick comments (2)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (2)
425-447
: Reduce duplication: prefer Packages over PackageCache and stream directory enumeration.
- Use Directory.EnumerateDirectories to avoid loading huge lists.
- Skip PackageCache tools when the corresponding Packages//Editor/MCPForUnityTools exists to prevent duplicate installs.
Example approach:
- Keep searchRoots ordered: Assets → Packages → PackageCache.
- When iterating PackageCache folders, map “com.pkg@x.y.z” → “com.pkg” and skip if Packages/com.pkg/Editor/MCPForUnityTools exists.
449-451
: Remove unused skippedCount or log it.It’s incremented but never used.
- int copiedCount = 0; - int skippedCount = 0; + int copiedCount = 0; ... - { - skippedCount++; - continue; - } + { + continue; + } ... - skippedCount++;Also applies to: 487-489, 525-526
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Dependencies/Models.meta
(1 hunks)MCPForUnity/Editor/Dependencies/PlatformDetectors.meta
(1 hunks)MCPForUnity/Editor/Helpers/ServerInstaller.cs
(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- MCPForUnity/Editor/Dependencies/Models.meta
- MCPForUnity/Editor/Dependencies/PlatformDetectors.meta
🔇 Additional comments (2)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (2)
61-64
: Good: run tool sync post-install.Decoupling tool sync from server version ensures updates on domain reload.
695-698
: Good: run tool sync after rebuild.Ensures rebuilt server includes latest project tools.
if (needsUpdate) | ||
{ | ||
// Get all .py files (excluding __init__.py) | ||
var pyFiles = Directory.GetFiles(folder, "*.py") | ||
.Where(f => !Path.GetFileName(f).Equals("__init__.py", StringComparison.OrdinalIgnoreCase)); | ||
|
||
// Skip folders with no .py files | ||
if (!pyFiles.Any()) | ||
{ | ||
skippedCount++; | ||
continue; | ||
} | ||
|
||
bool copyFailed = false; | ||
foreach (var pyFile in pyFiles) | ||
{ | ||
string fileName = Path.GetFileName(pyFile); | ||
string destFile = Path.Combine(destFolderSubdir, fileName); | ||
|
||
try | ||
{ | ||
File.Copy(pyFile, destFile, overwrite: true); | ||
copiedCount++; | ||
McpLog.Info($"Copied Unity project tool: {fileName} from {folderIdentifier} (v{sourceVersion})"); | ||
} | ||
catch (Exception ex) | ||
{ | ||
McpLog.Warn($"Failed to copy {fileName}: {ex.Message}"); | ||
copyFailed = true; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy one-level subdirectories too (aligns with server’s module discovery).
Only copying top-level .py files breaks directory-style tools (packages with init.py). Copy subdirs (excluding _skipDirs) one level deep.
bool copyFailed = false;
foreach (var pyFile in pyFiles)
{
string fileName = Path.GetFileName(pyFile);
string destFile = Path.Combine(destFolderSubdir, fileName);
try
{
File.Copy(pyFile, destFile, overwrite: true);
copiedCount++;
McpLog.Info($"Copied Unity project tool: {fileName} from {folderIdentifier} (v{sourceVersion})");
}
catch (Exception ex)
{
McpLog.Warn($"Failed to copy {fileName}: {ex.Message}");
copyFailed = true;
}
}
+
+ // Also copy one-level subdirectories (package-style tools), skipping caches/venvs
+ try
+ {
+ foreach (var sub in Directory.GetDirectories(folder, "*", SearchOption.TopDirectoryOnly))
+ {
+ string name = Path.GetFileName(sub);
+ if (_skipDirs.Any(s => name.Equals(s, StringComparison.OrdinalIgnoreCase)))
+ continue;
+ string destSub = Path.Combine(destFolderSubdir, name);
+ try
+ {
+ CopyDirectoryRecursive(sub, destSub);
+ }
+ catch (Exception ex)
+ {
+ McpLog.Warn($"Failed to copy subdirectory {name}: {ex.Message}");
+ copyFailed = true;
+ }
+ }
+ }
+ catch (Exception ex)
+ {
+ McpLog.Warn($"Failed enumerating subdirectories for {folderIdentifier}: {ex.Message}");
+ copyFailed = true;
+ }
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Helpers/ServerInstaller.cs around lines 478 to 508, the
current loop only copies top-level .py files which breaks package-style tools;
update the logic to also enumerate and copy one-level subdirectories under the
source folder (use Directory.GetDirectories(folder) and ignore any directory
names in the _skipDirs set), and for each allowed subdirectory create the
corresponding destination subdirectory (Path.Combine(destFolderSubdir,
subdirName)) then copy all files inside that subdirectory (preserving file names
and overwriting) while handling exceptions per file the same way as top-level
files (set copyFailed if any copy fails, increment copiedCount on success, log
successes and warnings). Ensure you still skip the whole source folder if there
are no top-level .py files and no allowed subdirectories to copy, and keep
existing skippedCount behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ok we force user to only have one py file. As it otherwise get complex to keep track of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JohanHoltby, understood! Keeping it to a single .py file per MCPForUnityTools folder is a reasonable simplification that avoids the complexity of tracking nested package structures. Thanks for clarifying the design decision.
✏️ Learnings added
Learnt from: JohanHoltby
PR: CoplayDev/unity-mcp#309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.047Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a lot of the discussions were on the issue (#308) when it should have been here, that's my bad 😅
I had, and still have, a lot of caveats with the approach, however, I like this feature a lot and I think we're really keen to get it in!
Original Feedback
- Revert the .meta file changes
git revert 571ca8c5de3d07da3791dad558677909a07e886d
- As mentioned in the issue comments, the
CUSTOM_TOOLS.md
needs to be updated explaining how this syncing works. Also describe best practices for adding multiple tools, and explain how they should treat withversion.txt
- Merge the latest code in
main
in your branch. A lot has changed since 🙂, and you may need to for the following feedback: - Code wise we can improve how this is implemented:
- Create a new service (see Services) folder for tool syncing
- Integrate it with the MCPServiceLocator class
- Use the functions from the new service in
PackageDetector.cs
andServerInstaller.cs
E.g. outline:
IToolSyncService.cs
namespace MCPForUnity.Editor.Services
{
public interface IToolSyncService
{
bool HasUpdatesPending(); // quick check used by PackageDetector
void SynchronizeTools(); // performs the scan/copy/cleanup
}
}
ToolSyncService
namespace MCPForUnity.Editor.Services
{
internal sealed class ToolSyncService : IToolSyncService
{
private readonly IPathResolverService _paths = MCPServiceLocator.Paths;
public bool HasUpdatesPending()
{
// The current code you have in ToolsVersionsChanged
}
public void SynchronizeTools()
{
// The current code you have in ServerInstaller
}
// Use any helper functions as you've added, they can stay private
}
}
MCPServiceLocator.cs - needs to be updated so your service can be referenced
public static class MCPServiceLocator
{
private static IToolSyncService _toolService;
public static IToolSyncService Tools => _toolService ??= new ToolSyncService();
public static void Register<T>(T implementation) where T : class
{
if (implementation is IToolSyncService tools)
_toolService = tools;
// …existing branches…
}
public static void Reset()
{
(_toolService as IDisposable)?.Dispose();
_toolService = null;
// …existing resets…
}
}
PackageDetector.cs
bool toolsNeedUpdate = MCPServiceLocator.Tools.HasUpdatesPending();
ServerInstaller.cs
MCPServiceLocator.Tools.SynchronizeTools();
Note: the above code structure is a suggestion!!! There are other solutions and interfaces you can create. The important thing is that this change should be a service and integrated with the service locator.
Updated Feedback
While typing the above, I finally figured out the key caveats I had with this PR: The hardcoded MCPForUnityTools
folders (I assume there can be more than 1 in a project) and version.txt
.
They're quite manual, I imagine those conventions work well for you but developers can be very particular about how they set up their project, so let's aim for a more general solution.
This is much more flexible - let's use a ScriptableObject that links to the Python tool files. Why is this better?
- No specific folder names, users can install wherever is convenient for them in the project.
- We can automatically hash the file when we list it, and use that as a way to automatically know if we need an update or not
- We can easily add more metadata about a Python file in the future if we need to
- A config is more predictable and easier to debug than folder names/structure + text files and other user conventions.
Attached is a quick attempt by Claude on how to implement the Scriptable Object solution, though I have not tested it.
There'll be no changes in PackageDetector.cs
this way, and ServerInstaller.cs
will only have simple changes:
// In ServerInstaller.cs - replace CopyUnityProjectTools() call
public static void EnsureServerInstalled()
{
// ... existing server installation code ...
// Sync Python tools using the service
string destToolsDir = Path.Combine(destSrc, "tools");
var syncResult = MCPServiceLocator.ToolSync.SyncProjectTools(destToolsDir);
if (syncResult.CopiedCount > 0)
{
McpLog.Info($"Synced {syncResult.CopiedCount} Python tool(s) to server");
}
}
Can you try the updated feedback? Of course, you still need to pull the latest code from main and merge in this branch, document these changes in CUSTOM_TOOLS.md
, and revert the .meta
files commit (probably do this first)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you revert this commit with the meta files change?
git revert 571ca8c5de3d07da3791dad558677909a07e886d
This reverts commit 571ca8c.
Thank you for the feedback! I agree that sounds like good suggestions. Especially the ScriptableObject parts. I will look in to it. I got a demo by 22/10 so will need to focus on that one. If I get time before I will do but other wise after. If some one else want this faster feel free to grab this PR or remake it from scratch. :) No hard feelings as longas we get the functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
596-629
: Fix PackageCache identifier collisions to prevent tool overwrites.The current logic doesn't check for
PackageCache
in the grandparent condition (lines 612-614), causing all PackageCache tools with the same structure to generate identical identifiers (e.g.,Editor_MCPForUnityTools
). This leads to collisions where tools from different packages overwrite each other.For example:
Library/PackageCache/com.pkg-a@1.0.0/Editor/MCPForUnityTools
→Editor_MCPForUnityTools
Library/PackageCache/com.pkg-b@2.0.0/Editor/MCPForUnityTools
→Editor_MCPForUnityTools
(collision!)Apply this diff to include the package name and distinguish PackageCache paths:
internal static string GetToolsFolderIdentifier(string toolsFolderPath) { try { - // Get parent directory name (e.g., "Editor" or package name) - DirectoryInfo parent = Directory.GetParent(toolsFolderPath); - if (parent == null) return "MCPForUnityTools"; - - // Walk up to find a distinctive parent (Assets/PackageName or Packages/PackageName) - DirectoryInfo current = parent; + // toolsFolderPath → .../Editor/MCPForUnityTools + var editorDir = Directory.GetParent(toolsFolderPath); // Editor + var packageDir = editorDir?.Parent; // Package folder + if (packageDir == null) return "MCPForUnityTools"; + + // Walk up to find root (Assets / Packages / PackageCache) + DirectoryInfo current = packageDir; while (current != null) { - string name = current.Name; - DirectoryInfo grandparent = current.Parent; - - // Stop at Assets, Packages, or if we find a package-like structure - if (grandparent != null && - (grandparent.Name.Equals("Assets", StringComparison.OrdinalIgnoreCase) || - grandparent.Name.Equals("Packages", StringComparison.OrdinalIgnoreCase))) + var parent = current.Parent; + if (parent != null && + (parent.Name.Equals("Assets", StringComparison.OrdinalIgnoreCase) || + parent.Name.Equals("Packages", StringComparison.OrdinalIgnoreCase) || + parent.Name.Equals("PackageCache", StringComparison.OrdinalIgnoreCase))) { - return $"{grandparent.Name}_{name}_MCPForUnityTools"; + return $"{parent.Name}_{current.Name}_MCPForUnityTools"; } - - current = grandparent; + current = parent; } - // Fallback: use immediate parent - return $"{parent.Name}_MCPForUnityTools"; + // Fallback: use package directory name + return $"{packageDir.Name}_MCPForUnityTools"; } catch { return "MCPForUnityTools"; } }
🧹 Nitpick comments (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (1)
463-489
: Consider deferring directory creation until needed.The destination subdirectory is created unconditionally (line 463) even when no update is required or no .py files exist. While this doesn't cause functional issues, deferring the creation until after the needsUpdate and pyFiles checks would be more efficient.
Apply this diff to defer directory creation:
string folderIdentifier = GetToolsFolderIdentifier(folder); activeFolderIdentifiers.Add(folderIdentifier); - // Create per-folder subdirectory in destToolsDir - string destFolderSubdir = Path.Combine(destToolsDir, folderIdentifier); - Directory.CreateDirectory(destFolderSubdir); + string destFolderSubdir = Path.Combine(destToolsDir, folderIdentifier); string versionTrackingFile = Path.Combine(destFolderSubdir, "version.txt"); // Read source version string sourceVersionFile = Path.Combine(folder, "version.txt"); string sourceVersion = ReadVersionFile(sourceVersionFile) ?? "0.0.0"; // Read installed version (tracked separately per tools folder) string installedVersion = ReadVersionFile(versionTrackingFile); // Check if update is needed (version different or no tracking file) bool needsUpdate = string.IsNullOrEmpty(installedVersion) || sourceVersion != installedVersion; if (needsUpdate) { // Get all .py files (excluding __init__.py) var pyFiles = Directory.GetFiles(folder, "*.py") .Where(f => !Path.GetFileName(f).Equals("__init__.py", StringComparison.OrdinalIgnoreCase)); // Skip folders with no .py files if (!pyFiles.Any()) { skippedCount++; continue; } + + // Create per-folder subdirectory now that we know we need it + Directory.CreateDirectory(destFolderSubdir); bool copyFailed = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/Editor/Helpers/ServerInstaller.cs
(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JohanHoltby
PR: CoplayDev/unity-mcp#309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
PR: CoplayDev/unity-mcp#309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/Editor/Helpers/ServerInstaller.cs
🔇 Additional comments (2)
MCPForUnity/Editor/Helpers/ServerInstaller.cs (2)
55-63
: Integration points look good.The tool synchronization is correctly invoked after server installation and rebuild operations, ensuring tools are up-to-date whenever the server changes.
Also applies to: 695-697
543-590
: Cleanup logic is correctly scoped.The stale folder removal correctly:
- Skips Python cache and virtual environment directories
- Only manages folders created by this feature (ending with
_MCPForUnityTools
)- Validates against active tool folders before deletion
This prevents accidental removal of server built-in tools or system directories.
Fixes Issue #308
This fixes the problem that adding a tool to the MCPForUnity Requires users to move around the py files and specific instructions on how to install so the MCPforUnity is editable.
Feature improvement
It out finds all folders called: MCPForUnityTools and check for a py file and an version.txt. On Domain reload if version.txt is updated the tool gets updated to the server folder e.g. C:\Users\UserName\AppData\Local\UnityMCP\UnityMcpServer\src\tools
This allows any one to make there asset MCP enabled by just installing MCPForUnity and add this folder to the tool. All would be automatic for end users.
Custom Tool folder structure
A good fodler structure on a custom tool would look like this:
CustomTool/
├── CustomTool.MCPEnabler.asmdef
├── CustomTool.MCPEnabler.asmdef.meta
├── ExternalAssetToolFunction.cs
├── ExternalAssetToolFunction.cs.meta
├── external_asset_tool_function.py
├── external_asset_tool_function.py.meta
├── version.txt
└── version.txt.meta
The asmdef is recommended to allow for dependency on MCPForUnity when MCP For Unity is installed:
asmdef example
{
"name": "CustomTool.MCPEnabler",
"rootNamespace": "MCPForUnity.Editor.Tools",
"references": [
"CustomTool",
"MCPForUnity.Editor"
],
"includePlatforms": [
"Editor"
],
"excludePlatforms": [],
"allowUnsafeCode": false,
"overrideReferences": false,
"precompiledReferences": [],
"autoReferenced": true,
"defineConstraints": [],
"versionDefines": [],
"noEngineReferences": false
}
CS files are left in the tools folder.
version.txt and the py file get copied.
Summary by CodeRabbit
New Features
Improvements